-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Onboarding] [Stack] Add Onboarding experience into Stack #204351
base: main
Are you sure you want to change the base?
Conversation
…ibana into onboarding-stack-add-plugin
…ibana into onboarding-stack-add-plugin
…ibana into onboarding-stack-add-plugin
cb0210a
to
af6b879
Compare
24542e5
to
735e3b5
Compare
2a9efd9
to
a6f3b8f
Compare
8a5eea6
to
1df4762
Compare
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @joemcelroy! Had an initial pass at the code and looks good, just have few questions about these two tests
@@ -18,14 +18,14 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { | |||
await pageObjects.common.navigateToApp('indexManagement'); | |||
}); | |||
|
|||
it('Navigates to the index details page from the home page', async () => { | |||
it.skip('Navigates to the index details page from the home page', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this break because of the changes or is it still wip? It seems like an important test to have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saarikabhasi 's Index Details locator PR will fix this, and it may merge first. but either way lets make sure this doesn't stay skipped.
await pageObjects.indexManagement.performIndexAction('flush'); | ||
await testSubjects.click('indexDetailsBackToIndicesButton'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my own understanding, why are these actions not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New searchIndex page has not flush function anymore :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't be updating these tests.
These should still be using the index management details page. But that will likely be handled by @saarikabhasi PR.
@@ -17,7 +17,7 @@ import { NewSearchIndexTemplate } from '../new_search_index_template'; | |||
|
|||
import { MethodApi } from './method_api'; | |||
|
|||
describe('MethodApi', () => { | |||
describe.skip('MethodApi', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats going on here, why are we skipping this?
@@ -49,7 +49,7 @@ export const buildBaseClassicNavItems = (): ClassicNavItem[] => { | |||
{ | |||
'data-test-subj': 'searchSideNav-Indices', | |||
deepLink: { | |||
link: 'enterpriseSearchContent:searchIndices', | |||
link: 'management:index_management', | |||
shouldShowActiveForSubroutes: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this, when you navigate to index management with classic nav you'll get a different page side nav.
shouldShowActiveForSubroutes: true, |
@@ -152,9 +148,9 @@ const mockNavLinks = [ | |||
url: '/app/elasticsearch/overview', | |||
}, | |||
{ | |||
id: 'enterpriseSearchContent:searchIndices', | |||
id: 'management:index_management', | |||
title: 'Indices', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this mock with the title for management:index_management
(Index Management
I think) so these test reflect reality.
OR we could override the title by setting a name:
property for this link in base_nav
so that it stays Indices
. But I think we wanted to show Index Management
for both classic and solution navs now. Confirm that and match the two links.
@@ -116,27 +115,21 @@ export const getNavigationTreeDefinition = ({ | |||
{ | |||
children: [ | |||
{ | |||
title: i18n.translate('xpack.enterpriseSearch.searchNav.kibana.indices', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment, we can either remove this title, to take the title from the management:index_management
deeplink or we should override the title in base_nav.tsx
to match this.
prepend('/app/management/data/index_management/') | ||
) || | ||
pathNameSerialized.startsWith(prepend('/app/elasticsearch/indices')) || | ||
pathNameSerialized.startsWith(prepend('/app/elasticsearch/start')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For stack '/app/elasticsearch/start'
is going to be active for Overview
instead of Index Management
so we'll want to remove this line. But I that can be done in the redirect PR @yansavitski is working on now.
} | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to useMemo this with a single locator. we can create a callback for the onClick event handler since it will only be used if the locator exists.
await a11y.testAppSnapshot(); | ||
}); | ||
|
||
it('index details - settings', async () => { | ||
await PageObjects.indexManagement.clickIndexDetailsTab('settings'); | ||
await PageObjects.searchIndexDetail.changeTab('settingsTab'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the tab key to be just settings
and not settingsTab
? (not required for this PR, but something we should note)
@@ -18,14 +18,14 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { | |||
await pageObjects.common.navigateToApp('indexManagement'); | |||
}); | |||
|
|||
it('Navigates to the index details page from the home page', async () => { | |||
it.skip('Navigates to the index details page from the home page', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saarikabhasi 's Index Details locator PR will fix this, and it may merge first. but either way lets make sure this doesn't stay skipped.
await pageObjects.indexManagement.performIndexAction('flush'); | ||
await testSubjects.click('indexDetailsBackToIndicesButton'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't be updating these tests.
These should still be using the index management details page. But that will likely be handled by @saarikabhasi PR.
@@ -82,9 +82,6 @@ export function IndexManagementPageProvider({ getService }: FtrProviderContext) | |||
async clickIndexAt(indexOfRow: number): Promise<void> { | |||
const indexList = await testSubjects.findAll('indexTableIndexNameLink'); | |||
await indexList[indexOfRow].click(); | |||
await retry.waitFor('details page title to show up', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not change this.
Summary
TODO
These changes are only targeting 9.0.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breaking
label should be applied in these situations.release_note:*
label is applied per the guidelines